Skip to content

fix: default addAcceptedScopesHeader and addAuthorizedScopesHeader to true - #512

Draft
dhensby wants to merge 1 commit into
node-oauth:masterfrom
dhensby:fix/authorize-scopes-header-defaults
Draft

dhensby wants to merge 1 commit into
node-oauth:masterfrom
dhensby:fix/authorize-scopes-header-defaults

Conversation

@dhensby

@dhensby dhensby commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Summary

addAcceptedScopesHeader and addAuthorizedScopesHeader are documented as defaulting to true, both on OAuth2Server and on AuthenticateHandler. Only OAuth2Server#authenticate() actually applied that default. AuthenticateHandler itself threw InvalidArgumentError: Missing parameter: addAcceptedScopesHeader whenever scope was set without them.

That breaks OAuth2Server#authorize() for any server configured with a scope option and no custom authenticateHandler. The constructor docs say server options are used as defaults for every method, so scope reaches the default AuthenticateHandler that AuthorizeHandler creates, but the header defaults don't. authorize() then throws synchronously, without even returning a rejected promise:

const server = new OAuth2Server({ model, scope: ['read'] });
await server.authenticate(request, response); // works
server.authorize(request, response);          // throws InvalidArgumentError: Missing parameter: `addAcceptedScopesHeader`

This PR applies the documented default (true when undefined) in the AuthenticateHandler constructor. That one change fixes authorize(), makes direct AuthenticateHandler use match its JSDoc, and keeps explicit false (or other) values as passed. The docs already describe this behaviour, so no doc changes are needed. OAuth2Server#authenticate() keeps its own explicit defaults, which are now redundant but harmless.

Linked issue(s)

None. Found while reviewing other open PRs.

Involved parts of the project

  • lib/handlers/authenticate-handler.js: constructor option defaults
  • The authorization endpoint (OAuth2Server#authorize() → AuthorizeHandler → default AuthenticateHandler)

Added tests?

Yes. Each of these fails on master with Missing parameter: … and passes here:

  • Server integration › authorize() › should set the scopes headers by default if scope is set on the server resolves with the code and sets X-Accepted-OAuth-Scopes / X-OAuth-Scopes.
  • AuthenticateHandler integration › constructor() has two tests that each default one option to true while keeping the other's explicit false.

The two tests that asserted the old Missing parameter errors are replaced by those default tests. biome check . is clean and the full mocha suite passes (465).

OAuth2 standard

Not affected. X-Accepted-OAuth-Scopes / X-OAuth-Scopes are informational, non-standard headers.

Reproduction

const OAuth2Server = require('@node-oauth/oauth2-server');
const { Request, Response } = OAuth2Server;

const server = new OAuth2Server({
  scope: ['read'],
  model: {
    getAccessToken: () => ({ user: {}, accessTokenExpiresAt: new Date(Date.now() + 10000), scope: ['read'] }),
    verifyScope: () => true,
    getClient: () => ({ id: 'c', grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] }),
    saveAuthorizationCode: (code) => code,
  },
});
const request = new Request({
  method: 'GET',
  headers: { Authorization: 'Bearer foo' },
  query: { state: 'xyz' },
  body: { client_id: 'c', response_type: 'code' },
});

server.authorize(request, new Response());
// master:  throws InvalidArgumentError: Missing parameter: `addAcceptedScopesHeader`
// this PR: resolves with the authorization code and sets both scopes headers

🤖 Generated with Claude Code

…` to `true`

Both options are documented as defaulting to `true`, but only
`OAuth2Server#authenticate()` applied that default. The AuthenticateHandler
itself threw `InvalidArgumentError: Missing parameter` when `scope` was set
without them.

As a result, `OAuth2Server#authorize()` threw synchronously for any server
configured with a `scope` option (which the constructor docs say is used as
the default for the other methods) and no custom `authenticateHandler`,
because the default AuthenticateHandler it creates received the server's
`scope` but not the header options.

The AuthenticateHandler now applies the documented default, so
`authorize()` works and sets both scopes headers, and explicit `false`
values are still respected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dhensby
dhensby requested a balanced review from Copilot October 15, 2026 14:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@dhensby
dhensby requested a balanced review from Copilot October 15, 2026 14:38
@jankapunkt jankapunkt added this to the 5.4 milestone Oct 15, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants